Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option for Zopfli iteration count #640

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

LegionMammal978
Copy link
Contributor

@LegionMammal978 LegionMammal978 commented Aug 3, 2024

This PR extends the --zopfli argument with an optional iteration count. In my case, I have a bunch of very small images (a few kB or less), and I often like to use hundreds of iterations to squeeze off the last several bytes. (I know that this crate isn't intended for brute-force optimization, but I've found that some of its transformations and filter strategies can be more creative than zopflipng.) But this is also useful in the opposite direction, for allowing Zopfli compression on large images where 15 iterations would be prohibitive.

src/main.rs Fixed Show resolved Hide resolved
src/main.rs Fixed Show fixed Hide fixed
Copy link
Collaborator

@AlexTMjugador AlexTMjugador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice PR, thanks! I tested the changes and they work as described, so after the Clippy lints are addressed, I think we can move forward with merging it. The ARM CI build failures are unrelated to this PR and thus not a concern for it.

@AlexTMjugador AlexTMjugador added the R-Pending documentation update Issues or pull requests whose resolution should be followed by doc updates (user manual, etc.) label Aug 3, 2024
@andrews05
Copy link
Collaborator

Thanks for doing this @LegionMammal978, it does feel a bit overdue 😅

I always thought about adding this but never did because a) nobody had asked for it and b) I couldn't decide how best to implement it. As you mention, optional parameters are a bit weird and I'm concerned the require_equals will inevitably trip some users up. Overloading --zc may indeed be a better solution, though care would have to be taken to ensure it's capped at 12 for libdeflate only. Or of course a new parameter such as --zi. Do you have any thoughts @AlexTMjugador?

@ace-dent
Copy link

ace-dent commented Aug 4, 2024

+1 for being able to control iterations. Nice :-)

Is the use of NonZeroU64 in place of NonZeroU8 necessary? Does it impact win32 compatibility?

@andrews05
Copy link
Collaborator

@ace-dent win32 should be fine, zopfli-rs is already using U64 so the change is just bringing oxipng in line.

That said, it is technically a breaking change. I don't think we're ready to jump to v10 at this point, @LegionMammal978 could we hold off on that part for now?

@ace-dent
Copy link

ace-dent commented Aug 4, 2024

@andrews05 - thanks.
It's a shame that -i is taken, or we could have had parity with advdef.
I'm not sure why adding a value after -Z / -zopfli is a breaking change?... If we ensure the absence of the parameter keeps the historic default value, would that work?

Alternatively, I prefer --zi than overloading --zc. :-)

@andrews05
Copy link
Collaborator

andrews05 commented Aug 4, 2024

The breaking change is the API changing from U8 to U64.

Adding a value to --zopfli isn't breaking, but the fact that the value is optional means that it can only be used in the form --zopfli=100 and not --zopfli 100, otherwise the value is ambiguous. I worry that the inconsistency with the syntax of all other arguments, coupled with the = syntax being less commonly used anyway, could cause confusion with users.

I'm also leaning towards --zi over --zc.

@LegionMammal978
Copy link
Contributor Author

Alright, I've backed out the NonZeroU64 change and added a separate --zi argument which requires --zopfli.

src/cli.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@andrews05 andrews05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks @LegionMammal978. I'll just wait to hear @AlexTMjugador's thoughts on the changes (he may also need to fix the failing workflow before we can merge).

@AlexTMjugador
Copy link
Collaborator

AlexTMjugador commented Aug 7, 2024

Overloading --zc may indeed be a better solution, though care would have to be taken to ensure it's capped at 12 for libdeflate only. Or of course a new parameter such as --zi. Do you have any thoughts @AlexTMjugador?

After giving this some thought, I concur it's probably best to go with --zi for specifying iterations 👍

Newer versions of Zopfli can also operate in a "do iterations until no compression gains are achieved" mode, which can be useful for people looking into brute-forcing their way to marginally better compression, and that is more straightforward to expose in OxiPNG through additional dedicated command-line switches than by overloading already existing ones. So setting this precedent here is a good idea.

Is the use of NonZeroU64 in place of NonZeroU8 necessary?

I don't think this is necessary for OxiPNG at the moment either. Upstream Zopfli began accepting such high iteration counts to support the mode outlined above (after all, there is little difference in real usage between unbounded iterations and 264 - 1 iterations), but I don't think there's any practical purpose for them besides that. 255 iterations is more than enough room to brute-force higher compression for PNGs.

(he may also need to fix the failing workflow before we can merge).

Yeah, I will need to get down to this sooner than later... Linux musl AArch64 nightly Rust builds have an annoying habit of breaking down due to esoteric, supposedly-fixed symbol resolution heisenbugs every some months. <opinion>I guess this is the sort of things one can expect when foundational software tools are maintained with scarce resources by skilled volunteers, while billions of dollars are poured into dubious "industry-changing" "AI-powered" projects by stakeholders who neglect to understand that "AI" also needs reliable and ongoing maintenance on toolchains and other "solved problems" to even work.</opinion>

AlexTMjugador added a commit that referenced this pull request Aug 7, 2024
This is required to get PR #640 and further work on the repository
moving.
@AlexTMjugador AlexTMjugador merged commit 0f24120 into shssoichiro:master Aug 7, 2024
11 checks passed
@LegionMammal978
Copy link
Contributor Author

Newer versions of Zopfli can also operate in a "do iterations until no compression gains are achieved" mode

Would you happen to have links to these? The main Zopfli repo hasn't really changed in functionality since 2016, and it's hard to figure out whether any reimplementations have improved on the basic algorithm.

255 iterations is more than enough room to brute-force higher compression for PNGs.

One might think so. But for the small PNGs I mentioned above, I can often recover another dozen bytes or so by bumping it up to 500 or 1000 iterations. Past that, every order of magnitude in the iteration count tends to recover 1–5 bytes. So huge iteration counts can be useful if you really want to squeeze an image.

@AlexTMjugador
Copy link
Collaborator

AlexTMjugador commented Aug 9, 2024

Would you happen to have links to these? The main Zopfli repo hasn't really changed in functionality since 2016, and it's hard to figure out whether any reimplementations have improved on the basic algorithm.

Sure! OxiPNG uses the most popular Rust library that reimplements Zopfli, whose docs can be read here (the specific documentation for the option that allows such behavior is here). The compression code is very similar to the original Zopfli C code, but it adds some API niceties on top of what's available in the C version, such as support for streaming compression and the aforementioned compression mode.

One might think so. But for the small PNGs I mentioned above, I can often recover another dozen bytes or so by bumping it up to 500 or 1000 iterations. Past that, every order of magnitude in the iteration count tends to recover 1–5 bytes. So huge iteration counts can be useful if you really want to squeeze an image.

That's interesting, I definitely wasn't patient enough to notice that emergent behavior at very high iteration counts. It looks like there's a reasonable use case for it then. Thanks for sharing!

@andrews05
Copy link
Collaborator

We can certainly make the change to U64 in v10. Along with adding support for iterations_without_improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R-Pending documentation update Issues or pull requests whose resolution should be followed by doc updates (user manual, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants